-
-
Notifications
You must be signed in to change notification settings - Fork 226
Make get_differential_vars type stable #2698
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Yeah, the more the marrier. https://github.com/SciML/OrdinaryDiffEq.jl/blob/master/test/interface/mass_matrix_tests.jl is probably a good test to slam some around. |
Turns out that lots of these are still not inferrable--even with this PR, going from Rosenbrock DAE AD tests, the move from sol = solve(prob_mm, Rodas5P(), reltol = 1e-8, abstol = 1e-8) to using ADTypes: AutoForwardDiff
sol = @inferred solve(prob_mm, Rodas5P(autodiff=AutoForwardDiff(chunksize=3)), reltol = 1e-8, abstol = 1e-8) already runs into an issue, since the type of That said, even though this PR apparently doesn't do enough for the existing test suite, it is already helping with the MWE and therefore probably my code base. |
It's fine to sprinkle some |
OK, so the quirk in the test I was looking at turns out to be that for M = Diagonal([1.0, 1.0, 0.0])
roberf = ODEFunction(rober, mass_matrix = M)
M = [1.0 0 0
0 1.0 0
0 0 0]
roberf = ODEFunction(rober, mass_matrix = M) With a full matrix, inference's best guess at We currently get
So, for example, a matrix like |
I believe the proper definition is that the number of algebraic variables is determined by the size of the null space of M. If But I couldn't figure out how to calculate this easily so the diagonal cases were handled and any case where someone couldn't figure out the differential variables but needed it turned into an error. In lots of cases it's not necessary to calculate so it ended up being an okay solution, with a few special cases like dense. But yes... I don't know, I think in general you probably need to QR factorize the M and then take the julia> M = [1 1 0; 1 1 0; 0 0 0]
3×3 Matrix{Int64}:
1 1 0
1 1 0
0 0 0
julia> qr(M).R
3×3 Matrix{Float64}:
-1.41421 -1.41421 0.0
0.0 0.0 0.0
0.0 0.0 0.0 That definition isn't quite right either. |
OK, that makes sense--I had the sense my solution might be too easy, otherwise it would have already been there. I'll revert it to keep the previous behavior, I think For at least some of the tests, I can just change to explicitly using a |
M = [1.0 0 0 | ||
0 1.0 0 | ||
0 0 0] | ||
M = Diagonal([1.0, 1.0, 0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AayushSabharwal does MTK use Diagonal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it generates a Matrix{Float64}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's update that
Results of adding To my confusion: in the Rosenbrock tests, inference works for:
In the OrdinaryDiffEqBDF tests, I find that even with a specified chunksize the precise algorithm type cannot be fully inferred, so for those I added With those tests now marked as broken, I believe the tests will run to completion now, where they didn't before. In the InterfaceII Mass Matrix tests, most use dense mass matrices, for which the solution will not be inferrable; for the diagonal cases it seems to work as far as I tested, but the final example isn't inferrable even if I specify the chunksize: OrdinaryDiffEq.jl/test/interface/mass_matrix_tests.jl Lines 313 to 338 in 60e6ba1
|
Okay, let's get what we got in, mark other things as |
That's pretty much where it stands now, I think, so we can merge this and close #2594. The full test suite now runs with a handful of |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Solves #2594 , based on #2594 (comment).
Question on testing
It seems like a good idea to add a test that would have caught #2594; my attempt would be to add
@inferred
in a couple places. WouldOrdinaryDiffEq.jl/lib/OrdinaryDiffEqRosenbrock/test/dae_rosenbrock_ad_tests.jl
Lines 31 to 42 in cc958fb
or
OrdinaryDiffEq.jl/lib/OrdinaryDiffEqBDF/test/dae_ad_tests.jl
Lines 14 to 37 in cc958fb
be a good place for these? Should this get tested with all the implicit solvers where the mass-matrix DAE formulation is allowed?